Skip to content

Log instead of warn when registering an ID prefix that already exists #75

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 23, 2025
Merged

Log instead of warn when registering an ID prefix that already exists #75

merged 1 commit into from
Jun 23, 2025

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Jun 23, 2025

Copilot AI review requested due to automatic review settings June 23, 2025 08:16
@Marenz Marenz requested a review from a team as a code owner June 23, 2025 08:16
@github-actions github-actions bot added the part:id Affects the id module label Jun 23, 2025
Copilot AI reviewed Jun 23, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the behavior when registering an ID prefix that is already in use by replacing a call to warn with a logging message.

  • Replace warnings.warn with a logging call
  • Introduce a logger instance for proper logging

src/frequenz/core/id.py Outdated
f"Prefix '{str_prefix}' is already registered. "
"ID prefixes must be unique.",
stacklevel=2,
_logger.warn(
Copy link

Copilot AI Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider replacing the deprecated '_logger.warn' with '_logger.warning' to align with current best practices in the logging module.

Suggested change
_logger.warn(
_logger.warning(

Copilot uses AI. Check for mistakes.
@Marenz Marenz force-pushed the warn branch 2 times, most recently from c46afdd to 667c3c1 Compare June 23, 2025 08:20
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests labels Jun 23, 2025
llucax
llucax previously approved these changes Jun 23, 2025
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put in the commit/release notes that the change was made so downstream projects don't need to add a warning exception in pytest.

@Marenz
Log instead of warn when registering an ID prefix that already exists
ac73bc5 
Using `warn()` in the `BaseId` class caused problems for downstream
projects, as it required them to add an exception for exactly this
warning. Instead, we now log a warning using the `logging` module.

Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz enabled auto-merge June 23, 2025 08:30
@Marenz Marenz disabled auto-merge June 23, 2025 08:40
@Marenz Marenz merged commit f5576e1 into frequenz-floss:v1.x.x Jun 23, 2025
5 checks passed
@Marenz Marenz deleted the warn branch June 23, 2025 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@llucax llucax llucax left review comments

@florian-wagner-frequenz florian-wagner-frequenz Awaiting requested review from florian-wagner-frequenz florian-wagner-frequenz is a code owner automatically assigned from frequenz-floss/python-sdk-team

Assignees

No one assigned

Labels

part:docs Affects the documentation part:id Affects the id module part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Marenz @llucax